Add more btests to replace "unit" tests #417
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The new btest connects to Broker endpoints and then publishes data on one endpoint while receiving on the other.
The `base_fixture` is quite heavyweight and some tests, like the `multipath` test suite, uses the fixture only for `id_by_value`. Pushing this functionality to a new fixture reduces unnecessary dependencies on the base fixture, allowing us to make more aggressive changes to it in the future (or to remove it entirely).
With the new btest, we cover all the APIs tested in the `publisher`, `subscriber` and `status_subscriber` "unit" tests. Since the old tests are actually system tests that no longer add any value over the new btest suites, we can now get rid of them.
The legacy `master.test.cc` file really only tests `put_unique`. Since we already have a btest for that, this no longer adds any value. Furthermore, the test really is a system test that relies on a complex fixture setup that we want to get rid of.
The legacy `integration` test suite covers forwarding and unpeering. We have new btest-based system tests for both now and thus no longer need the legacy test.
The `channel` test suite relies on the low-level `base_fixture` setup but really doesn't need actors. This new implementation simply dispatches to producers and consumers directly from the list of outgoing messages that we keep anyways to simulate loss rates, without needing to go through the extra abstraction of actors.
The core actor unit test suite really only tests forwarding between peers. However, we already have added a btest covering that, so this test no longer serves a purpose.
timwoj
approved these changes
Jul 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since we have access to
btestin Broker for a while now, it's about time we replace some of our "unit" tests withbtest-based tests instead. Historically, we have used a special fixture setup in Broker for tests that need multiple Broker endpoints to get around having to orchestrate multiple processes. This is really just working around the limitations we had at the time, i.e., only having access to a unit test framework but really needed to have system tests in place.This isn't just about swapping tools, because the setup for these "unit" tests:
peerandlisten, which means we test against internal implementations details of Broker and some user-facing APIs remain untested